perf: optimize CI workflow and Dockerfiles for faster builds#31
perf: optimize CI workflow and Dockerfiles for faster builds#31
Conversation
- Remove QEMU setup from build-and-test (only builds amd64) - Add setup job: fetches s6-overlay version once and computes PR-aware matrix (skip PHP 8.3 on PRs, test 8.4 + 8.2 only) - Add BuildKit cache mounts for apt/apk in both Dockerfiles - Extract retry/backoff logic to shared extras/retry.sh script, replacing ~50 lines of duplicated inline retry loops - Enable BuildKit for v1 in test-build.sh (required for cache mounts)
There was a problem hiding this comment.
Pull request overview
Optimizes CI runtime and Docker build performance by reducing redundant work in the GitHub Actions workflow and enabling BuildKit caching / shared retry logic in Dockerfiles.
Changes:
- Adds a
setupjob to compute a PR-aware PHP version list and fetch the latest s6-overlay version once for reuse. - Enables BuildKit cache mounts for package managers in both Dockerfiles and enables BuildKit for v1 local builds.
- Extracts Dockerfile retry/backoff logic into a shared
extras/retry.shhelper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/docker-ci.yml |
Adds setup job outputs and reuses them; removes QEMU from amd64-only build-and-test job. |
Dockerfile.v1 |
Adds shared retry helper and BuildKit cache mounts. |
Dockerfile.v2 |
Adds shared retry helper and BuildKit cache mounts; replaces inline retry loops. |
extras/retry.sh |
New shared retry wrapper used by Dockerfiles. |
extras/test-build.sh |
Enables BuildKit for v1 local build to support cache mounts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/docker-ci.yml
Outdated
| PHPVERSION=${{ matrix.php-version }} | ||
| BASEOS=${{ matrix.php-base }} | ||
| S6_OVERLAY_VERSION=${{ steps.s6-version.outputs.version }} | ||
| S6_OVERLAY_VERSION=${{ needs.setup.outputs.s6-version }} |
There was a problem hiding this comment.
Same issue here: needs.setup.outputs.s6-version uses an output key with a hyphen, which can break expression parsing. Use bracket notation (needs.setup.outputs['s6-version']) or rename the output key.
| S6_OVERLAY_VERSION=${{ needs.setup.outputs.s6-version }} | |
| S6_OVERLAY_VERSION=${{ needs.setup.outputs['s6-version'] }} |
| run: | | ||
| if [ "${{ github.event_name }}" = "pull_request" ]; then | ||
| echo 'php-versions=["8.4","8.2"]' >> $GITHUB_OUTPUT | ||
| echo "::notice::PR detected — testing PHP 8.4 + 8.2 only (skipping 8.3)" | ||
| else | ||
| echo 'php-versions=["8.4","8.3","8.2"]' >> $GITHUB_OUTPUT | ||
| fi |
There was a problem hiding this comment.
The PR-only matrix reduction to 8.4 + 8.2 is currently applied only to matrix.php-version, but any hard-coded matrix.include entries can still introduce 8.3 jobs even on pull_request runs. To get the intended CI savings, generate the full matrix (including the trixie v2 extras) in the setup job, or make the include list conditional on the same PR check.
.github/workflows/docker-ci.yml
Outdated
| S6_OVERLAY_VERSION="$(curl -s https://api.github.com/repos/just-containers/s6-overlay/releases/latest | jq -r .tag_name)" | ||
| echo "version=${S6_OVERLAY_VERSION}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
This step fetches the latest s6-overlay tag without -f/error handling or authentication. If the API call is rate-limited or fails, jq -r .tag_name can return an empty/null value that then propagates into the Docker build args. Consider using curl -fSL (and validating the tag is non-empty) and/or authenticating with Authorization: Bearer ${{ github.token }} to reduce flakiness.
| S6_OVERLAY_VERSION="$(curl -s https://api.github.com/repos/just-containers/s6-overlay/releases/latest | jq -r .tag_name)" | |
| echo "version=${S6_OVERLAY_VERSION}" >> $GITHUB_OUTPUT | |
| set -euo pipefail | |
| RAW_RESPONSE="$(curl -fSLs \ | |
| -H "Authorization: Bearer ${{ github.token }}" \ | |
| "https://api.github.com/repos/just-containers/s6-overlay/releases/latest")" | |
| S6_OVERLAY_VERSION="$(printf '%s\n' "${RAW_RESPONSE}" | jq -r '.tag_name')" | |
| if [ -z "${S6_OVERLAY_VERSION}" ] || [ "${S6_OVERLAY_VERSION}" = "null" ]; then | |
| echo "Error: Failed to determine latest s6-overlay version from GitHub API." >&2 | |
| exit 1 | |
| fi | |
| echo "version=${S6_OVERLAY_VERSION}" >> "$GITHUB_OUTPUT" |
.github/workflows/docker-ci.yml
Outdated
| outputs: | ||
| php-versions: ${{ steps.matrix.outputs.php-versions }} | ||
| s6-version: ${{ steps.s6.outputs.version }} |
There was a problem hiding this comment.
The job output names php-versions and s6-version contain hyphens, which makes them awkward/unsafe to reference via dot-notation in GitHub Actions expressions (it can be parsed as subtraction). Rename these outputs to use _ (e.g., php_versions, s6_version) or switch all references to bracket notation (e.g., needs.setup.outputs['php-versions']).
.github/workflows/docker-ci.yml
Outdated
| matrix: | ||
| variant: [v1, v2] | ||
| php-version: ['8.4', '8.3', '8.2'] | ||
| php-version: ${{ fromJson(needs.setup.outputs.php-versions) }} | ||
| php-type: [fpm, cli, apache] |
There was a problem hiding this comment.
This expression references a setup output key containing a hyphen (php-versions). With dot-notation (needs.setup.outputs.php-versions) GitHub Actions can interpret this as subtraction and fail expression evaluation. Use bracket notation (needs.setup.outputs['php-versions']) or rename the output to an identifier-safe name (e.g., php_versions).
.github/workflows/docker-ci.yml
Outdated
| PHPVERSION=${{ matrix.php-version }} | ||
| BASEOS=${{ matrix.php-base }} | ||
| S6_OVERLAY_VERSION=${{ steps.s6-version.outputs.version }} | ||
| S6_OVERLAY_VERSION=${{ needs.setup.outputs.s6-version }} |
There was a problem hiding this comment.
needs.setup.outputs.s6-version references an output key with a hyphen; with dot-notation this can be parsed incorrectly and break the build args. Use bracket notation (needs.setup.outputs['s6-version']) or rename the output to an identifier-safe name (e.g., s6_version).
| S6_OVERLAY_VERSION=${{ needs.setup.outputs.s6-version }} | |
| S6_OVERLAY_VERSION=${{ needs.setup.outputs['s6-version'] }} |
| RUN --mount=type=cache,target=/var/cache/apt,sharing=locked,id=apt-$BASEOS \ | ||
| --mount=type=cache,target=/var/lib/apt/lists,sharing=locked,id=aptlists-$BASEOS \ | ||
| --mount=type=cache,target=/var/cache/apk,sharing=locked,id=apk-$BASEOS \ | ||
| if [ "$BASEOS" = "bookworm" ]; then \ |
There was a problem hiding this comment.
The cache mount for /var/cache/apk won’t be used while the Alpine branch installs with apk add --no-cache (which disables the package cache). Either drop the apk cache mount, or switch to a cache-friendly apk invocation so this mount actually improves rebuild times.
Dockerfile.v2
Outdated
| RUN if [ "$BASEOS" = "trixie" ] || [ "$BASEOS" = "bookworm" ]; then \ | ||
| RUN --mount=type=cache,target=/var/cache/apt,sharing=locked,id=apt-$BASEOS \ | ||
| --mount=type=cache,target=/var/lib/apt/lists,sharing=locked,id=aptlists-$BASEOS \ | ||
| --mount=type=cache,target=/var/cache/apk,sharing=locked,id=apk-$BASEOS \ |
There was a problem hiding this comment.
The cache mount for /var/cache/apk likely won’t have any effect because the Alpine branch uses apk add --no-cache, which avoids writing to the apk cache directory. Either remove the apk cache mount (to reduce complexity) or adjust the apk install flags to take advantage of the cache mount.
| --mount=type=cache,target=/var/cache/apk,sharing=locked,id=apk-$BASEOS \ |
extras/retry.sh
Outdated
| @@ -0,0 +1,21 @@ | |||
| #!/bin/sh | |||
| # Retry a command with exponential backoff | |||
There was a problem hiding this comment.
The comment says "exponential backoff", but the implemented sleep (SLEEP_TIME=$((5 * ATTEMPT))) is linear. Either update the comment to match the behavior, or adjust the sleep formula to be exponential so the documentation isn’t misleading.
| # Retry a command with exponential backoff | |
| # Retry a command with linear backoff |
- Rename hyphenated output keys to underscores (php_versions, s6_version) - Generate full matrix (including trixie includes) in setup job so PR matrix reduction correctly skips PHP 8.3 everywhere - Harden s6-overlay API call with -f flag, auth token, and null check - Remove useless apk cache mount (apk --no-cache bypasses cache dir) - Fix retry.sh comment: backoff is linear, not exponential
Summary
setupjob that fetches s6-overlay version once (instead of 30+ times) and computes a PR-aware matrix (skips PHP 8.3 on PRs, testing 8.4 + 8.2 only) — saves ~50-80 min/PR--mount=type=cachefor apt/apk package caches in both Dockerfiles — faster rebuilds on cache hitsextras/retry.sh, replacing ~50 lines of inline retry loops across both Dockerfilestest-build.sh(required for cache mounts)Test plan
./extras/test-build.sh v1 8.3-fpm-bookworm— builds successfully./extras/test-build.sh v2 8.3-fpm-alpine— builds successfullydocker run --rm php-docker:8.3-fpm-alpine-v2 php -m— all 73 extensions load